Skip to content

[mypyc] Move API tables to static_data.c#21183

Open
p-sawicki wants to merge 2 commits intopython:masterfrom
p-sawicki:move-api-tables-to-static_data
Open

[mypyc] Move API tables to static_data.c#21183
p-sawicki wants to merge 2 commits intopython:masterfrom
p-sawicki:move-api-tables-to-static_data

Conversation

@p-sawicki
Copy link
Copy Markdown
Collaborator

lib-rt API tables are defined as static variables in lib-rt headers. This means that each translation unit gets its own independent instance of these variables.

That becomes a problem in multi-file compilation mode when using BytesWriter.write, as this function is translated to CPyBytesWriter_Write by mypyc which is defined in bytes_writer_extra_ops.c. With multi-file compilation, bytes_writer_extra_ops.c is compiled as its own translation unit and gets linked with the C extension compiled from python files.

The C extension TU copies the librt.strings capsule contents into the global table but because it's static this is not visible in the table in the bytes_writer_extra_ops.c TU, which stays zero-initialized. This results in a seg fault with the following call chain:

CPyBytesWriter_Write -> CPyBytesWriter_EnsureSize -> LibRTStrings_ByteWriter_grow_buffer_internal -> LibRTStrings_API[5]-> oops all zeros

To fix this, declare the tables as extern and define them in static_data.c so there's only one global version of each variable.


#ifdef MYPYC_EXPERIMENTAL

void *LibRTStrings_API[LIBRT_STRINGS_API_LEN] = {0};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now all the API tables will always be included in each build, which increases the memory usage of each module a little. It's not a big deal right now, but this is not scalable if we add many librt submodules in the future. Could we instead have librt_vecs_static.c etc. and add them as dependencies?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants